Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove double-hasing of typeid for handle #10699

Merged
merged 1 commit into from
Nov 23, 2023
Merged

remove double-hasing of typeid for handle #10699

merged 1 commit into from
Nov 23, 2023

Conversation

DasLixou
Copy link
Contributor

Objective

There is a double hash for the typeid for asset handles. (see this for more info)

Solution

  • Remove the second hash of the typeid

@DasLixou
Copy link
Contributor Author

@bushrat011899 can they be removed?

@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Performance A change motivated by improving speed, memory usage or compile times C-Code-Quality A section of code that is hard to understand or change labels Nov 22, 2023
@JMS55 JMS55 requested a review from cart November 22, 2023 18:33
@bushrat011899
Copy link
Contributor

bushrat011899 commented Nov 22, 2023 •
edited
Loading

I just checked over your change again with a coffee, I believe that should work, with the only consequence being that an AssetId will have the same hash as it's Handle, and likewise for the untyped version. I don't know if that's a problem of any kind, just thought I'd note it here.

mockersf
mockersf approved these changes Nov 22, 2023
View reviewed changes
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's an issue that they share an hash, they are typed enough and should not mix

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 23, 2023
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 23, 2023
Merged via the queue into bevyengine:main with commit 960f6e9 Nov 23, 2023
27 checks passed
@DasLixou DasLixou deleted the remove_unnecessary_doublehash branch November 23, 2023 05:43
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@DasLixou
remove double-hasing of typeid for handle (bevyengine#10699)
2ea13e7 
# Objective

There is a double hash for the typeid for asset handles. (see
[this](bevyengine#10695 (comment))
for more info)

## Solution

- Remove the second hash of the typeid
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

@cart cart Awaiting requested review from cart

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DasLixou @bushrat011899 @alice-i-cecile @mockersf